Fixes #299 #300
Fixes #299 #300
Conversation
Does not override GIT_SSH_COMMAND when not needed.
|
|
|
Any chances this one will be merged? |
|
Please merge! This would allow hashicorp/terraform#28968 to be closed! |
| } | ||
|
|
||
| env = append(env, strings.Join(sshCmd, " ")) | ||
| cmd.Env = env | ||
| } |
There was a problem hiding this comment.
Please expand/unfold the lines above so that you can see the whole setupGitEnv function--it's not long.
The only place sshCmd is modified, and would therefore need to be written back to the environment, is if sshKeyFile != "" (line 263). Apart from that case, there's currently no reason to initialize a default sshCmd as in these lines from above (259-261):
if len(sshCmd) == 0 {
sshCmd = []string{gitSSHCommand + "ssh"}
}
If you feel like there might be other types of modifications/mutations in the future (besides sshKeyFile), and you want to make it relatively clear/easy how to add them, then I'd recommend adding a variable like hasSshCmdBeenModified bool. E.g.,
var hasSshCmdBeenModified = false
if len(sshCmd) == 0 {
// Ensure a default command is initialized in case there are any subsequent modifications
sshCmd = []string{gitSSHCommand + "ssh"}
}
if sshKeyFile != "" {
sshCmd = addKeyModification(sshCmd, sshKeyFile)
hasSshCmdBeenModified = true
}
if shouldDoModificationB {
sshCmd = doModificationB(sshCmd)
hasSshCmdBeenModified = true
}
if shouldDoModificationC {
sshCmd = doModificationC(sshCmd)
hasSshCmdBeenModified = true
}
if hasSshCmdBeenModified {
// Don't append unless there was an actual modification.
// The value of sshCmd might be the temporary default that was prepared
// for modifications but didn't end up being needed.
env = append(env, strings.Join(sshCmd, " "))
}
There was a problem hiding this comment.
...or maybe:
baseSshCmdForModification = []string{gitSSHCommand + "ssh"}
if len(sshCmd) == 0 {
// Ensure a default command is initialized in case there are any subsequent modifications
sshCmd = baseSshCmdForModification
}
if sshKeyFile != "" {
sshCmd = addKeyModification(sshCmd, sshKeyFile)
}
if shouldDoModificationB {
sshCmd = doModificationB(sshCmd)
}
if shouldDoModificationC {
sshCmd = doModificationC(sshCmd)
}
if sshCmd != baseSshCmdForModification {
env = append(env, strings.Join(maybeModifiedSshCmd, " "))
}
There was a problem hiding this comment.
This would save having to export GIT_SSH_COMMAND in the environment regularly and fixes hashicorp/terraform#28968. This has been open for a bit, and merging would benefit at least a few of us.
|
@nywilken @claire-labry @kmoe @dpowley Any chance to give this some love? I'd much appreciate the merge or at least a comment why this cannot be merged. |
|
@claire-labry @picatz You were the latest who merged their branches to the current master. Any chance that you can endorse this one? |
Does not override GIT_SSH_COMMAND when not needed. See #299 .